Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-1635741: Port _datetime extension module to multiphase initialization (PEP 489) #30522

Closed
wants to merge 2 commits into from

Conversation

CharlieZhao95
Copy link
Contributor

@CharlieZhao95 CharlieZhao95 commented Jan 11, 2022

Port _datetime extension module to multiphase initialization

https://bugs.python.org/issue1635741

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@CharlieZhao95

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@shihai1991
Copy link
Member

@CharlieZhao95 Thanks for your contirbute. You can use ./python -m test test_datetime -v -R 3:3 to catch the failure of CI.

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Jan 11, 2022

  1. Please sign the CLA (See the devguide for more information).
  2. Most work related to bpo-1635741 has been put on hold by the SC (see SC communications as of Feb 2021). See also Victor's and my comment on bpo-1635741: port _ctypes extension module to multiphase initialization  #30525

Thank you for your interest in improving CPython! :)

@CharlieZhao95
Copy link
Contributor Author

CharlieZhao95 commented Jan 13, 2022

  1. Please sign the CLA (See the devguide for more information).
  2. Most work related to bpo-1635741 has been put on hold by the SC (see SC communications as of Feb 2021). See also Victor's and my comment on [bpo-1635741: port _ctypes extension module to multiphase initialization  bpo-1635741: port _ctypes extension module to multiphase initialization  #30525](bpo-1635741: port _ctypes extension module to multiphase initialization  #30525)

Thank you for your interest in improving CPython! :)

Thanks for your reply :) Indeed, it is better to convert global state to module state first. I will work on issue https://bugs.python.org/issue40077 (convert static types to heap types) and fix reference leak promblem caused by static types in this PR later.

@erlend-aasland
Copy link
Contributor

erlend-aasland commented Jan 13, 2022

Thanks for your reply :) Indeed, it is better to convert global state to module state first. I will work on issue https://bugs.python.org/issue40077 (convert static types to heap types) and fix reference leak promblem caused by static types in this PR later.

Sounds good. But the steps needed should be approximately like this:

  1. Discuss the changes with the module maintainer/code owner (for example a topic on Discourse, or a bpo issue for this specific module). Explain the rationale and reasoning behind the change. Continue with the subsequent steps if the module maintainer approves.
  2. Lay out an implementation plan. Remember that there may be performance bottle necks when retrieving module state; care must be taken not to introduce a performance regression.
  3. Apply Argument Clinic to the modules types; it will help retrieving module state later in the process. Perhaps this is already done; I did not go to the gemba to look 🙂
  4. Establish a global module state
    • Create a module state struct and add a (temporary) instance on the module stack
    • Move global variables to module state struct one by one. (This is where issue 40077 happens!)
  5. Convert global module state to module state; take care that no performance regression is introduced => benchmark
  6. Apply multi-phase initialisation

IMO, this process should be spread out over several PRs, to make it easier to review. (I try to keep the diffs of my PRs below 200 lines of code.) It is IMO of importance that all of this happens in the course of a single alpha development phase. You cannot use a beta phase for changes like this. 3.11 beta starts in about 4 months time from now, IIRC.

I suggest you take a shot at it on your own fork, so you'll be ready for when the SC has approved we can continue with these changes 😃

@erlend-aasland
Copy link
Contributor

Oh, thanks for signing the CLA! 🚀 😃

@CharlieZhao95 CharlieZhao95 marked this pull request as draft January 18, 2022 07:02
@erlend-aasland
Copy link
Contributor

FYI, PEP-687 was just accepted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants